Review physics module code quality #1223
Conversation
Documents bugs, missing features, and improvements found during code review of shards/modules/physics: P0 (Critical): - Memory leaks in body/constraint cleanup - ApplyForceAt logic bug (&&/|| operator) - Typo in assertion macro P1 (Missing Features): - No raycasting/shape queries - Missing shape types (cylinder, mesh, heightfield) - Missing constraint types (hinge, cone, etc) - Body sleeping disabled P2 (Improvements): - Soft body param detection broken - Hardcoded system limits - Various minor issues
There was a problem hiding this comment.
Pull request overview
This PR documents findings from a comprehensive code review of the shards/modules/physics module, categorizing issues by priority (P0-Critical, P1-Missing Features, P2-Improvements). The documentation provides detailed descriptions, code examples, impact analysis, and suggested fixes for each identified issue, serving as a roadmap for physics module improvements.
Key changes:
- Documents 5 critical bugs including memory leaks and logic errors
- Identifies 5 missing feature categories (raycasting, shapes, constraints, sleeping, character controllers)
- Lists 7 improvement opportunities ranging from broken parameter detection to empty help text
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/physics-issues/P0-critical-bugs.md | Documents 5 critical bugs including memory leaks in body/constraint cleanup, ApplyForceAt logic error, and assertion macro typo |
| docs/physics-issues/P1-missing-features.md | Catalogs missing physics capabilities: raycasting/queries, additional shape types, constraint types, body sleeping, and character controller support |
| docs/physics-issues/P2-improvements.md | Lists 7 lower-priority improvements including broken soft body parameter detection, hardcoded system limits, and missing documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```cpp | ||
| void cleanup(SHContext *context) { | ||
| if (_instance.node && _instance.node->node) { | ||
| auto &bodyNode = _instance->node; |
There was a problem hiding this comment.
The variable name in the code example is inconsistent. Line 14 checks if (_instance.node && _instance.node->node) but line 15 declares auto &bodyNode = _instance->node;. This should be auto &bodyNode = _instance.node; to match the condition check.
| auto &bodyNode = _instance->node; | |
| auto &bodyNode = _instance.node; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #1223 +/- ##
==========================================
- Coverage 77.74% 77.73% -0.01%
==========================================
Files 386 386
Lines 59575 59575
==========================================
- Hits 46315 46313 -2
- Misses 13260 13262 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claude can you open the issues listed in the PR .md files |
|
Claude finished @sinkingsugar's task —— View job Opening Issues from Physics Module ReviewStatus: Unable to Create IssuesI've analyzed all the documentation files and found 17 issues to create, but I don't have permission to create GitHub issues via the Summary of Issues FoundP0 - Critical Bugs (5 issues):
P1 - Missing Features (5 issues):
P2 - Improvements (7 issues):
Next StepsTo create these issues, you can either:
|
Documents bugs, missing features, and improvements found during code review of shards/modules/physics:
P0 (Critical):
P1 (Missing Features):
P2 (Improvements):